Skip to content

Propagate enable_profiling through ProcessedContainerSettings#483

Open
jhiemstrawisc wants to merge 3 commits into
Reed-CompBio:mainfrom
jhiemstrawisc:fix-profiling
Open

Propagate enable_profiling through ProcessedContainerSettings#483
jhiemstrawisc wants to merge 3 commits into
Reed-CompBio:mainfrom
jhiemstrawisc:fix-profiling

Conversation

@jhiemstrawisc

Copy link
Copy Markdown
Collaborator

ProcessedContainerSettings.from_container_settings() rebuilds the processed container settings from the raw ContainerSettings, but it never copied the enable_profiling field, so the processed settings always used the dataclass default of False. run_container_singularity() reads the flag off the processed settings (config.enable_profiling), so the profiling branch never ran: no peer cgroup was created, cgroup_wrapper.sh never executed, and no usage-profile.tsv was written, regardless of the user's containers.enable_profiling config value.

This regressed in #390, which moved enable_profiling from a top-level config field to the nested container settings and added the field to both ContainerSettings and ProcessedContainerSettings, but missed wiring it through the conversion in from_container_settings().

Add the missing assignment, plus a regression test asserting that enable_profiling survives the full config-parsing path.

ProcessedContainerSettings.from_container_settings() rebuilds the processed
container settings from the raw ContainerSettings, but it never copied the
enable_profiling field, so the processed settings always used the dataclass
default of False. run_container_singularity() reads the flag off the processed
settings (config.enable_profiling), so the profiling branch never ran: no peer
cgroup was created, cgroup_wrapper.sh never executed, and no usage-profile.tsv
was written, regardless of the user's containers.enable_profiling config value.

This regressed in Reed-CompBio#390, which moved enable_profiling from a top-level config
field to the nested container settings and added the field to both
ContainerSettings and ProcessedContainerSettings, but missed wiring it through
the conversion in from_container_settings().

Add the missing assignment, plus a regression test in test_config.py asserting
that enable_profiling survives the full config-parsing path.
@jhiemstrawisc jhiemstrawisc requested review from agitter and ntalluri June 8, 2026 21:46

@agitter agitter left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code changes look good. I have not tested it yet, which one of us may want to do before merging. I tried testing in CHTC but hit other problems described in #459 (review)

@ntalluri

Copy link
Copy Markdown
Collaborator

I updated my code on my current CHTC set up with these changes. I am currently waiting for my jobs to get scheduled to test if this fixed the bug.

@ntalluri

Copy link
Copy Markdown
Collaborator

I accidentally approved this. I still need to test this on CHTC, I realized that the SPRAS Docker image I was using wasn't including this code change so it still looked like a bug.

@jhiemstrawisc

Copy link
Copy Markdown
Collaborator Author

I'm testing this now, and I've found a few more subtle issues -- don't merge yet!

subprocess.run rejects capture_output=True together with an explicit
stderr argument (capture_output is shorthand for stdout=PIPE, stderr=PIPE).
In the profiling/cgroup branch of run_container_singularity this raised
"ValueError: stdout and stderr arguments may not be used with
capture_output." on every Apptainer run with enable_profiling=true.

Set stdout=PIPE and stderr=STDOUT explicitly so the container's stderr is
merged into the captured stdout, preserving the original intent of
capturing combined output via proc.stdout.
The profiling file is written next to raw-pathway.txt inside the reconstruct
rule, but it was never declared as a Snakemake output. With a shared
filesystem this is harmless, but under shared-fs-usage: none (e.g. HTCondor)
Snakemake only transfers declared outputs back from execute nodes, so the
profiling data was created on the EP and then discarded.

Declare usage-profile.tsv as an additional reconstruct output, gated on
enable_profiling and the singularity/apptainer framework so the output is
only required when it is actually produced (run_container_singularity),
matching the existing warn-only behavior for profiling under docker.
@jhiemstrawisc

Copy link
Copy Markdown
Collaborator Author

The most recent commits address two other issues I ran into while testing. The first fixes a runtime error that I think I introduced at some point related to the way I used subprocess to invoke apptainer on the EP. The second declares the profiling file as an output so it's transferred back to the AP. This second point was previously hidden by the fact that the executor used to transfer the EP's entire output director rather than individual files (which was itself a bug because it caused Snakemake to re-run completed jobs).

At this point I've tested profiling end to end and this works for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants